Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instructions to compile test2json without local go #226

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

g-gaston
Copy link
Contributor

With the current instructions you actually need a local go installation. Even if the go source is downloaded, a go binary is needed to compile test2json, even if it's a different version than the test2json one.

This PR adds instructions to download a temporary go binary, compile test2json from it and delete it.

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

Can you help me better understand the use case that is better served by this change to the docs?

I believe even in the updated version of the docs the cmd/test2json path refers to a Go source checkout. Since a Go source checkout is needed in both cases, it seems like it might be easier for most use cases to simply compile all the binaries from the source checkout.

Technically the change is correct. You could use a different version of Go to build test2json, but I don't understand in what cases that would be preferable. I guess it might be faster to download than to build all the binaries?

These instructions are mostly intended to be guidelines, to help someone understand the dependencies needed to run gotestsum. I don't expect them to capture every use case. Maybe we could document this as an alternative approach?

@g-gaston g-gaston force-pushed the compile-test2json-without-go branch from 682d507 to 8755c22 Compare January 4, 2022 10:47
@g-gaston
Copy link
Contributor Author

g-gaston commented Jan 4, 2022

Thank you for the PR!

Can you help me better understand the use case that is better served by this change to the docs?

I believe even in the updated version of the docs the cmd/test2json path refers to a Go source checkout. Since a Go source checkout is needed in both cases, it seems like it might be easier for most use cases to simply compile all the binaries from the source checkout.

Technically the change is correct. You could use a different version of Go to build test2json, but I don't understand in what cases that would be preferable. I guess it might be faster to download than to build all the binaries?

These instructions are mostly intended to be guidelines, to help someone understand the dependencies needed to run gotestsum. I don't expect them to capture every use case. Maybe we could document this as an alternative approach?

@dnephin of course!

You are right, the source code is needed in both cases. However, with the current docs, we need a go binary already installed and in path in order to compile the test2json binary. If we tried to run those commands in a fresh machine without go, the command from line 14 would fail. And if the machine already has go installed, why not use the second method (which is significantly simpler)? Maybe I'm missing something here and my whole hypothesis is wrong :)

I think I didn't explain correctly my goal. I didn't intend for the docs to show how to compile test2json from a different go version, but to show how to compile and install test2json in a machine without a previous go installation.

If you prefer to not include this in the docs because it's a edge case or because that wasn't the intent of this doc, that's cool with me. I just thought that the idea was to show how to use gotestsum without an actual dependency on a go local installation. Or if you prefer me to leave the current explanation as is and add a new section, I can also do that 🙂

As for my usecase, I was trying to build an image for running tests in CI but I didn't want to install go just to install test2json.

By the way, I just updated the PR with a couple env vars I missed that are necessary for go build

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! That's a good point about the old version. I missed that those instructions also required an existing Go binary.

If I remember correctly, the purpose of these docs was to show how to build test2json so that it can be copied to another machine for CI.

I think the edits you made are generally a good improvement, but I think saying "without a Go install" is a bit misleading. I would categorize the curl step that downloads Go as installing it. Once Go is downloaded there's no reason to build test2json for the local machine, since you could run go tool test2json. Building the binary is more for distributing to other places without having to distribute the whole Go toolchain.

I've suggested a few small edits, but otherwise looks great!

docs/running-without-go.md Outdated Show resolved Hide resolved
docs/running-without-go.md Outdated Show resolved Hide resolved
docs/running-without-go.md Outdated Show resolved Hide resolved
…installation

Co-authored-by: Daniel Nephin <dnephin@gmail.com>
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@g-gaston
Copy link
Contributor Author

@dnephin done, PTAL 🙂

@dnephin dnephin merged commit 661b091 into gotestyourself:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants